Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Migrate WasmModule construction to top-level factory functions. #94

Merged
merged 1 commit into from
Nov 6, 2022
Merged

Migrate WasmModule construction to top-level factory functions. #94

merged 1 commit into from
Nov 6, 2022

Conversation

modulovalue
Copy link
Contributor

See #92 step 2.

The following are some TODOs as a note to myself:

  • the circular dependency between runtime.compile and WasmModule should be broken by having the owner be a function that receives the pointer and returns an owner and a generic return type.
  • the async version for constructing a WasmModule is not doing anything that the sync version isn't doing and could be removed. It seems to act as a reminder for providing an async construction scheme in the future?

@liamappelbe
Copy link
Contributor

What's the rationale for switching to top-level factory functions?

The async version is there for when we add web support. Wasmer doesn't have a built in way of compiling asynchronously, but on the web that's the main recommended way.

@modulovalue
Copy link
Contributor Author

What's the rationale for switching to top-level factory functions?

By routing all ways to instantiate a wasmer based WasmModule through these two top level factory functions we'll be able to make everything else private in that file to explicitly exclude it from the public API. (we could use parts, but that would add a dependency on wasmer to wasm_api.dart).

Together with wasm_api.dart (from the other PR) we'll have a clear picture about what constitutes the public API of this package: wasm_api.dart (all sealed interfaces) and two functions for creating new wasm modules. This will give us more freedom to work on the internals without accidentally breaking users of this package.

@liamappelbe
Copy link
Contributor

Sounds reasonable. How would we provide a different implementation of these functions for the web? Just delegate to impl functions defined in conditionally imported scripts?

liamappelbe
liamappelbe suggested changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@liamappelbe liamappelbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the change log?

@modulovalue
Copy link
Contributor Author

Conditional imports would be one way.

However, there are some subtleties that make conditional imports suboptimal in my opinion. One of which is that they aren't really supported all that well by the dart analyzer. (But they are the most user friendly way in the sense that users won't have to care about any implementation details.)

@modulovalue
Copy link
Contributor Author

changelog updated (and version bumped)

@modulovalue
Migrate WasmModule construction to top-level factory functions.
d25ab1c 
update changelog


migrate brotli api


add newline to changelog
@modulovalue
Copy link
Contributor Author

I've rebased on main and squashed the commits.
@liamappelbe please take a look. I don't have write permissions to merge or is there something else that you'd like me to do?

@liamappelbe liamappelbe merged commit 748d950 into dart-archive:main Nov 6, 2022
@modulovalue modulovalue deleted the single_entrypoint branch November 7, 2022 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liamappelbe liamappelbe liamappelbe approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@modulovalue @liamappelbe